fix: bignumber format by time formatter #1307
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/superset/superset-ui/7ARZGfTidB5zxyFxhjZdTGbG4n9T |
Codecov Report
@@ Coverage Diff @@
## master #1307 +/- ##
==========================================
+ Coverage 29.94% 30.46% +0.52%
==========================================
Files 485 489 +4
Lines 9819 10129 +310
Branches 1644 1796 +152
==========================================
+ Hits 2940 3086 +146
- Misses 6644 6797 +153
- Partials 235 246 +11
Continue to review full report at Codecov.
|
| config: { | ||
| type: 'SelectControl', | ||
| freeForm: true, | ||
| label: t('Timestamp format'), |
There was a problem hiding this comment.
Can we name it Date format for consistency with other charts?
Also, instead of adding a checkbox to control visibility, why don't we just make this select button clearable?
There was a problem hiding this comment.
We need an explicit way to tell the user whether to use number formatter or datetime formatter
junlincc
changed the title
| @@ -135,7 +135,7 @@ describe('BigNumber', () => { | |||
| }, | |||
| }; | |||
| const transformed = transformProps(propsWithDatasource); | |||
| expect(transformed.formatNumber(transformed.bigNumber)).toStrictEqual('1.23'); | |||
| expect(transformed.headerFormatter(transformed.bigNumber)).toStrictEqual('1.23'); | |||
There was a problem hiding this comment.
Can you add a test for timestamp?
There was a problem hiding this comment.
I will push a separate PR after this. Thanks!
| @@ -146,7 +147,9 @@ export default function transformProps(chartProps: BigNumberChartProps) { | |||
| }); | |||
| } | |||
|
|
|||
| const formatNumber = getNumberFormatter(yAxisFormat); | |||
| const headerFormatter = headerFormatSelector | |||
There was a problem hiding this comment.
Can we rename headerFormatSelector to isFormatTimestamp or something like that? It's not obvious that it's a boolean value which is true if formatter is for timestamp
There was a problem hiding this comment.
I will push a separate PR after this. Thanks!
🏆 Enhancements
closes: apache/superset#16221
Support timestamp format in BigNumber viz
Test instruction: